Client reports download status and download details #341
Conversation
When the client is downloading a package it will report the download status, and report download details (bytes per second and percent download) periodically.
michel-laterman
changed the title
Add feedback from review, not expected to pass until spec changes are made
client/internal/packagessyncer.go
Outdated
| } | ||
| // start the download reporter | ||
| detailsReporter := newDownloadReporter(downloadReporterDefaultInterval, packageLength) // TODO set interval | ||
| detailsReporter.report(ctx, status, s.reportStatuses) |
There was a problem hiding this comment.
Is reportStatuses safe to be called concurrently? The docs don't say it, so I am not sure. Note that reportStatuses calls user-supplied SetLastReportedStatuses which also is not documented to be safe for concurrent call. If you look at the only implementation in InMemPackagesStore I think it is indeed not safe to call concurrently.
| if p.packageLength > 0 { | ||
| downloadPercent = downloaded / p.packageLength * 100 | ||
| } | ||
| status.DownloadDetails = &protobufs.PackageDownloadDetails{ |
There was a problem hiding this comment.
I am a bit worried that status is a struct owned by someone else and we modify it from our goroutine here, while status's other fields are also simultaneously being modified by the owner. If the code is accidentally modified in the future to modify the same fields we will have a problem.
Can we instead change updateFn to accept the DownloadPercent and DownloadBytesPerSecond as parameters and eliminate the status parameter from downloadReporter? This way the responsibility for getting concurrency correctly will be sole responsibility of the caller of downloadReporter.report instead of being a shared responsibility.
| if p.packageLength > 0 { | ||
| downloadPercent = downloaded / p.packageLength * 100 | ||
| } | ||
| _ = updateFn(ctx, downloadPercent, bps) |
There was a problem hiding this comment.
Nit: can we change updateFn to take a struct { DownloadPercent float, Bps float } so that it is clear which of the 2 floats is which.
| if err != nil { | ||
| return fmt.Errorf("failed to install/update the package %s downloaded from %s: %v", pkgName, file.DownloadUrl, err) | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| func (s *packagesSyncer) getDownloadDetailsFn(pkgName string) func(context.Context, float64, float64) error { |
There was a problem hiding this comment.
I think this func updates the download details, so perhaps a more fitting name is updateDownloadDetails?
| DownloadPercent: percent, | ||
| DownloadBytesPerSecond: rate, | ||
| } | ||
| return s.reportStatuses(ctx, true) |
There was a problem hiding this comment.
This will call SetLastReportedStatuses. I think to be safe we need to document SetLastReportedStatuses and require that it must be prepared to be called concurrently.
Reading the current code, it appears we are NOT calling it concurrently. We call once at the start of downloadFile and then periodically, while detailsReporter is active. However, it still makes me uncomfortable, since these calls are made from different goroutines and it is very easy to accidentally change the code so that we end up calling reportStatuses concurrently.
Just to future-proof it I think it make sense to caution implementors of SetLastReportedStatuses and ask them to guard any mutations they have against concurrent call. We can also demonstrate it on InMemPackagesStore implementation (should be trivial to add a mutex that protects the updates).
What do you think?
Add implementation of downloading status along with download details details as described in open-telemetry/opamp-spec#206
When the client is downloading a package it will report the download status, and report download details (bytes per second and percent download) periodically.